Skip to content

[skip-changelog] Add query parameters to urls generated by lib commands #2034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jan 17, 2023
Merged

[skip-changelog] Add query parameters to urls generated by lib commands #2034

merged 3 commits into from
Jan 17, 2023

Conversation

MatteoPologruto
Copy link
Contributor

Please check if the PR fulfills these requirements

See how to contribute

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)

What kind of change does this PR introduce?

Code enhancement

What is the new behavior?

Three different query parameters have been added to the urls generated by lib commands like lib install and lib upgrade. The query parameters are:

  • install: which is selected when a library is installed through lib install;
  • upgrade: which is selected when a library is upgraded through lib upgrade;
  • depends: which is selected when a library dependency is installed.

Does this PR introduce a breaking change, and is titled accordingly?

No

@MatteoPologruto MatteoPologruto added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Jan 17, 2023
@MatteoPologruto MatteoPologruto self-assigned this Jan 17, 2023
@codecov
Copy link

codecov bot commented Jan 17, 2023 •
edited
Loading

Codecov Report

Base: 36.57% // Head: 36.55% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (ccc8c7f) compared to base (b3e8f8a).
Patch coverage: 13.88% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2034      +/-   ##
==========================================
- Coverage   36.57%   36.55%   -0.02%     
==========================================
  Files         230      230              
  Lines       19353    19366      +13     
==========================================
+ Hits         7079     7080       +1     
- Misses      11439    11450      +11     
- Partials      835      836       +1
Flag Coverage Δ
unit 36.55% <13.88%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
arduino/cores/packagemanager/profiles.go 0.00% <0.00%> (ø)
arduino/resources/index.go 40.20% <0.00%> (ø)
commands/daemon/daemon.go 0.00% <ø> (ø)
commands/instances.go 40.68% <0.00%> (ø)
commands/lib/download.go 0.00% <0.00%> (ø)
commands/lib/install.go 0.00% <0.00%> (ø)
commands/lib/upgrade.go 0.00% <0.00%> (ø)
internal/cli/lib/download.go 0.00% <0.00%> (ø)
internal/cli/lib/install.go 0.00% <0.00%> (ø)
arduino/httpclient/httpclient.go 58.33% <40.00%> (-2.39%) ⬇️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@MatteoPologruto MatteoPologruto marked this pull request as ready for review January 17, 2023 09:34
@umbynos umbynos requested a review from cmaglie January 17, 2023 11:39
umbynos
umbynos suggested changes Jan 17, 2023
View reviewed changes
Copy link
Contributor

@umbynos umbynos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some suggestions

umbynos
umbynos approved these changes Jan 17, 2023
View reviewed changes
Copy link
Contributor

@umbynos umbynos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🏁

@MatteoPologruto MatteoPologruto merged commit c666277 into arduino:master Jan 17, 2023
@MatteoPologruto MatteoPologruto deleted the query-parameter branch January 17, 2023 15:06
@cmaglie
Copy link
Member

cmaglie commented Jan 18, 2023 •
edited
Loading

I see a couple of problems with this PR:

  • it changes the signature of the function commands.LibraryDownload and commands.LibraryInstall, this is a breaking change that is not documented.
  • all public functions in package commands are made to strictly follow the gRPC API. If we need to add a parameter in any of the public functions of command.* we must first change the gRPC API and rebuild it otherwise, we allow the CLI to bypass the gRPC API. In fact, this implementation works only in the CLI, there is no way to access this feature from the gRPC API (so it will not work in the Arduino IDE or any other gRPC client).

@MatteoPologruto MatteoPologruto mentioned this pull request Jan 18, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@umbynos umbynos umbynos approved these changes

@cmaglie cmaglie Awaiting requested review from cmaglie

Assignees

@MatteoPologruto MatteoPologruto

Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MatteoPologruto @cmaglie @umbynos